fix(security): authorize the invoked operation against the authenticated principal - #2217
fix(security): authorize the invoked operation against the authenticated principal#2217cb1kenobi wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request strengthens the security and authorization model of the operations API, specifically within chooseOperation. It ensures that the authenticated principal is always enforced and prevents body-supplied parameters (like nested hdb_user or parsed_sql_object) from bypassing authorization checks. Additionally, it fixes a critical bug in processAST where permission denials (which are objects lacking a length property) were not correctly blocked. The review feedback suggests a minor optimization in chooseOperation to avoid polluting the top-level request object with parsed_sql_object when executing nested SQL searches, as it is only consumed by direct SQL operations.
|
Reviewed; no blockers found. |
|
We'll have some overlap with #2202 on your point 4 -- more details to come shortly! |
kriszyp
left a comment
There was a problem hiding this comment.
Nice, glad to be reviewing the auth on SQL, looks like some good improvements. It looks like there are a few things to address (and maybe needs a rebase).
🤖 Reviewed with Codex
19c0026 to
2df2eb2
Compare
|
Rebased onto What changed in the reconciliation
Testing — One open question flagged inline on the nested-read thread: the nested check re-applies gate 1, making non-SQL export slightly stricter than SQL export for roles with a restrictive 🤖 Generated by Barber AI (Opus 4.8) |
2df2eb2 to
1235c38
Compare
8f56329 to
14ac88f
Compare
…ted principal chooseOperation handed verifyPerms the caller-supplied search_operation, making both halves of the permission question — principal and tables — body-controlled. Rebased onto main, which independently landed the SQL operations-allowlist check, apiOperation token-scope threading (#2176, #2260), and the processAST denial fix (#2202). This layers the principal/subject hardening on top and closes the gaps the original PR had deferred: - Principal comes from authentication: a nested hdb_user is overwritten, never backfilled. - search_operation stands in as the permission subject only for the export operations that consume it, must be an object, and must name a supported export operation (search_by_value/hash/conditions/sql) — a primitive, {}, or an unsupported op is a request-time 400. - One verifyPerms call cannot authorize both the outer export and its nested query: the outer op is authorized first, then the nested search is authorized additively against its real search handler and the authenticated principal, so a role granted export_local but not the underlying read is denied (previously granted at verifyPerms gate 2 before any table check). - SQL export is routed through verifyPerms too, so its requires_su gate is enforced exactly as on the non-SQL path — SQL is no longer a way around it. (main's SQL branch checked only the allowlist + AST, so a non-super user could export via a SQL search_operation.) Regression cover in integrationTests/security/choose-operation-authz.test.ts: NESTED-NOSQL now asserts denial, NESTED-OP covers empty/invalid nested operations, POSITIVE-NOSQL proves a permitted non-SQL export still completes; and the northwind SQL-export cases now assert the requires_su denial. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mallory's role sets an `operations` allowlist that excludes both add_user and export_local, so gate 1 (verifyOperationsAllowlist on the top-level op) denies her in both the fixed and a reverted dispatch — the two tests passed without exercising the routing fix at all. Switch both to TABLER, which declares no `operations`: gate 1 is a no-op, so the only barrier is that a nested `operation: 'sql'` must not route the outer op past its verifyPerms/requires_su check. Reverting the scoping fix now creates a super_user account (DISPATCH) or lets a requires_su export through (DISPATCH-SU), so the tests fail closed. Not EXPORTER for DISPATCH-SU: it lists export_local, which gate 2 grants, so it is legitimately allowed to export (200) and would not discriminate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1d406e7 to
f207347
Compare
kriszyp
left a comment
There was a problem hiding this comment.
I think one more thing to check on SQL writes/DML, but this looks good.
🤖 Reviewed with Codex
| } | ||
| } | ||
| // `json.operation` explicitly — the operation this dispatch already resolved, not a | ||
| // field read back off the request body. |
There was a problem hiding this comment.
Nested SQL is still authorized as the outer export plus AST table permissions, without applying the inner SQL/statement operation allowlist. For an ordinary authenticated role with operations: ['export_local'] and delete: true on a table—but no sql or delete operation grant—search_operation: { operation: 'sql', sql: 'DELETE ...' } passes the outer gate and AST check, then performs the deletion in the job before export output handling. The non-SQL path at server/serverHelpers/serverUtilities.ts:427 does apply its inner operation gate, so SQL is also less restrictive than the other nested searches. Please either restrict export SQL to SELECT, which best matches dataLayer/export.ts:327’s getRecords contract, or add the corresponding inner SQL/write-operation gate. A regression should submit a nested DELETE through an export-only role and verify the row remains.
(server/serverHelpers/serverUtilities.ts:368 is not part of this PR's diff — anchored to the nearest line this PR's diff can hold)
Fixes operation authorization in
chooseOperation: authorize the invoked operation against the authenticated principal, and stop a caller-suppliedsearch_operationfrom standing in as the permission subject.What this fixes
verifyPermsreads both halves of the permission question off the object it's handed — the principal fromhdb_userand the tables fromschema/database/table/records.chooseOperationused to hand itjson.search_operation, a caller-supplied field, making both halves body-controlled. Four issues, each with regression cover inintegrationTests/security/choose-operation-authz.test.ts:verifyPermsran againstjson.search_operation ?? jsonfor every operation, but the handler runs against top-leveljsonand onlydataLayer/export.tsconsumessearch_operation. A non-super user could send a privileged top-level operation with a benignsearch_operationand get authorized against the benign tables. Nowsearch_operationis the permission subject only forexport_local/export_to_s3; every other op is checked against top-leveljson.if (!hdb_user)), honoring a body-supplied one. Nowhdb_useris overwritten from the authenticated top-level principal unconditionally.parsed_sql_objectsmuggling — the export worker re-readsparsed_sql_object(carryingpermissions_checked) off the nested object, so a body-supplied one would execute an AST no check ever saw. It's now deleted at dispatch, forcing a re-parse + re-check in the worker.processAST's guard testedpermissionsCheck.length > 0, but a denial is aPermissionResponseObjectwith no.length, soundefined > 0was always false and denials executed. Fixed toif (permissionsCheck).Non-object
search_operationon an export op is now a 400 (was a wrapped 500), and the SQL AST check runs additively afterverifyPermsrather than as an exclusive branch.For the reviewer
server/serverHelpers/serverUtilities.ts(chooseOperation) is the core. The load-bearing invariant: the object passed toverifyPermsmust be the one the handler actually operates on.DESIGN.md(newchooseOperationsection) documents the three rules and why each is required.export_localtable check is currently unreachable — both export ops arerequires_suand every path returns before the nested table check (super_user early-returns; a role granted the op viaoperationsreturns at gate 2; anything else is refused earlier). So the nested-table substitution is inert today. This is intentional for this patch.Behavior change (release note)
Non-super users can no longer invoke
export_local/export_to_s3via a SQLsearch_operation. These operations arerequires_su, and NoSQL export already enforced that ("Operation 'export_local' is restricted to 'super_user' roles"). SQL-based export previously took the SQL-only branch and skippedverifyPerms, so a non-super user could invoke the privileged export by wrapping it in a SQL search — governed only by table read perms. Routing export throughverifyPermscloses that bypass and brings SQL export to parity with NoSQL export. Any non-super role that relied on SQL-export-without-export_localwill now receive a 403. Threenorthwindexport tests that encoded the old bypass were updated to assert therequires_sudenial (they now mirror the existing NoSQL export case; they could be consolidated).Deferred gaps (tracked, not fixed here)
Two pre-existing authorization gaps are left open deliberately — closing either changes authorization outcomes for existing role configurations, so each needs its own change + release note. Both are documented in
DESIGN.mdand pinned by regression tests so there's a test to flip:operationsallowlist bypasses table-level grants (non-SQL nested search) #2215 — a role grantedexport_local/export_to_s3viaoperationscan export tables it holds no grant on (non-SQL nested search reaches the handler with no table check).sqloperation bypasses the operations allowlist (gate 1) #2216 — a directsqloperation bypasses theoperationsallowlist (gate 1); authorized only by the AST table check.Verification
integrationTests/security/choose-operation-authz.test.ts— pins the denial shapes and the current boundaries (principal override, nested-shape 400, forged-AST rejection, and the two deferred-gap boundariesNESTED-NOSQL/ gate-1).processASTdenial-drop, with no other.length-style denial-drop anywhere in the tree. The multipass independently rediscovered the two deferred gaps above (now ticketed). One low cleanup nit surfaced:serverUtilities.ts:298sets a top-levelparsed_sql_objectthat's inert for the export path.Review coverage
processASTdenial-drop; no other.length-style denial-drop in the tree.agyfailed (stochastic print-mode hang; CLI retries + 2 manual attempts all timed out) — no perf/maintainability lens this run;cursor-composernot authenticated locally; the CLI's Claude domain-adjudicator leg couldn't run (claudenot on PATH) — covered instead by the separate multipass deep-review above.Unresolved review findings (test-only, non-blocking)
integrationTests/security/choose-operation-authz.test.ts: no end-to-end case proves the export worker's nested-SQL recheck can deny an unauthorized query. The allowed super-user path reachingCOMPLETEis covered, andunitTests/sqlTranslator/processAST.test.jsproves the object-shaped denial stops routing, but the full dispatch → principal-propagation → reparse → denial path isn't exercised together. Worth adding a non-super-user unauthorized-export-SQL case that asserts denial.